I think this give us exactly what we need. My only concern is that it sacrifices compile-type parameter checking, which is a substantial part of the value of gophercloud. I wonder if we could write something with strongly-typed arguments. If we were keen enough I'm pretty sure it could also be generated from base ListOpts argument.

It would be nice to be able to easily merge an existing ListOpts object with this

It would be nice to be able to easily merge an existing ListOpts object with this

or maybe that's a bad idea. Being able to merge a ListOpts with this would give the impression that you can have "this set of properties" OR "this property", while it will just merge the common properties instead.

I would be inclined to add a new struct explicitly for each resource which can use it, alongside the existing ListOpts. Maybe (ports|subnets|networks).ListOptsMulti? Little bit more boilerplate, but at least we wouldn't be accidentally adding support where we shouldn't.

we wouldn't be accidentally adding support where we shouldn't

Can you give me an example of what you mean?

I have added a bit of reflection. New now takes a struct (possibly, one of the existing ListOpts) and parses the field names in the q:"field-name" tags. And methods will reject fields that aren't defined in the base struct.

This gives a minimal (runtime) type check that may or may not add some safety.

also @mdbooth @dulek for review

and @spjmurray if you happen to pass by!

Yup, this looks pretty good to me.

we wouldn't be accidentally adding support where we shouldn't

Can you give me an example of what you mean?

Firstly I don't think we are adding support where we shouldn't because we need to implement the relevant interfaces for each point of use, specifically ToPortListQuery, ToNetworkListQuery, and ToSubnetListQuery. We can create one of these generic ListOpts, but we can't use it anywhere except these 3 specific places.

It feels a bit smelly to have this here. If we can successfully make it generic I would probably put it somewhere like openstack/networking/v2/networks/utils.go because it's specifically a neutron thing.

However, this is really an actual part of the API. I feel like it should live in https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/networks/requests.go alongside ListOpts. If we just created ListOptsMulti next to ListOpts which is identical except all its fields are slices instead of singletons then it would be compile-time type safe and very simple: basically just boilerplate. We could use something like this code to reduce the boilerplate and avoid implementation errors.

Ah yes, this is what we need!

Just for discussion purposes we could actually get this working today with something like:

import (
        "fmt"
        "net/url"

        "github.com/gophercloud/gophercloud"
        "github.com/gophercloud/gophercloud/openstack/networking/v2/ports"
)

type MyListOpts struct {
        ports.ListOpts

        Name []string `q:"name"`
}

func BuildCompositeQueryString(opts, base interface{}) (url.Values, error) {
        baseURL, err := gophercloud.BuildQueryString(base)
        if err != nil {
                return nil, err
        }

        optsURL, err := gophercloud.BuildQueryString(opts)
        if err != nil {
                return nil, err
        }

        baseQuery := baseURL.Query()

        for parameter, values := range optsURL.Query() {
                baseQuery.Del(parameter)

                for _, value := range values {
                        baseQuery.Add(parameter, value)
                }
        }

        return baseQuery, nil
}

func (opts MyListOpts) ToPortListQuery() (string, error) {
        values, err := BuildCompositeQueryString(opts, opts.ListOpts)
        return values.Encode(), err
}

func main() {
        opts := MyListOpts{
                ListOpts: ports.ListOpts{
                        Status: "detached",
                        Name: "ignored, hopefully",
                },
                Name: []string{
                        "port1",
                        "port2",
                },
        }

        query, err := opts.ToPortListQuery()
        if err != nil {
                fmt.Println("fail", err)
        }

        fmt.Println(query)
}

Kinda ugly though!

The obvious other way would be to have:

type MultiOpts struct {
   Name []string `q:"name"`
}

type ListOpts struct {
  Name string `q:"name"`

  MultiOpts MultiOpts
}